RangeControl: Add support for large 40px number input size #49105
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
richtabor
added
[Type] Enhancement
|
Size Change: +72 B (0%) Total Size: 1.44 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 34f8cf7. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5346384047
|
mirka
reviewed
Mar 16, 2023
packages/components/src/range-control/styles/range-control-styles.ts
Outdated
Show resolved
Hide resolved
packages/components/src/range-control/styles/range-control-styles.ts
Outdated
Show resolved
Hide resolved
|
Hey @richtabor , just checking if you need any help with this PR or if you put it on hold for any specific reasons? |
Sorry, just got caught up on other fronts. Feel free to take over if you'd like. |
|
Picked this PR up today, I'll wrap up the rest of the necessary changes 👍 |
Let us know when it's ready for a new review. 👍 |
|
I've taken a pass at all outstanding comments - let me know if there's any new or updated feedback! |
ciampo
reviewed
Jun 5, 2023
packages/components/src/range-control/styles/range-control-styles.ts
Outdated
Show resolved
Hide resolved
packages/components/src/range-control/styles/range-control-styles.ts
Outdated
Show resolved
Hide resolved
packages/components/src/range-control/styles/range-control-styles.ts
Outdated
Show resolved
Hide resolved
|
Okay, I've addressed the latest pass of feedback (sorry for the delayed turnaround). I think that should address everything, unless we want to do away with the increased width discussed here. Based on @jasmussen's answers (thanks for those!) it looks like we could go either way on that for now. |
|
I'll defer to Rich, but happy to provide a green check when things settle! |
mirka
approved these changes
Jun 19, 2023
| size = 'default', | ||
| }: Pick< NumberControlProps, 'size' > ) => { | ||
| const sizes = { | ||
| small: space( 16 ), |
There was a problem hiding this comment.
We can remove this because the small case will never happen.
There was a problem hiding this comment.
My bad on this one as well! This is also the result of a change that I suggested to clean up styles and types a bit, small is there to the sizes object has, as keys, all possible values that of size prop from NumberControl.
Feel free to make changes as needed :)
There was a problem hiding this comment.
Good point. That only got added when we decided to re-use the NumberControl prop type instead of typing this bit locally. Can remove the small case, but I had to get a little creative with the typing as a result. 24190be
Edit: this might be a moot point if we can scrip this logic altogether
Comment on lines
52
to
53
| /* TODO: remove after removing the __next40pxDefaultSize prop */ | ||
| ${ rootMinHeight } |
There was a problem hiding this comment.
Just a stylistic thing, but as a courtesy for when we actually have to remove the temporary __next* props, I find it cleaner to structure the code so that the ! __next case is already handled as the exception case.
For example:
Suggested change
| /* TODO: remove after removing the __next40pxDefaultSize prop */ | |
| ${ rootMinHeight } | |
| min-height: 40px; | |
| ${ deprecatedHeight }; |
where deprecatedHeight only handles the ! __next40pxDefaultSize case. The clean up will be easier this way. (Actual example)
There was a problem hiding this comment.
Apologies, I had actually suggested Chad to write code this way!
| // @todo: Refactor RangeControl with latest HStack configuration | ||
| // @wordpress/components/ui/hstack. | ||
| export const InputNumber = styled( NumberControl )` | ||
| display: inline-block; | ||
| font-size: 13px; | ||
| margin-top: 0; | ||
| width: ${ space( 16 ) } !important; | ||
| ${ inputNumberWidth }; |
There was a problem hiding this comment.
Would it be better/possible to use the __unstableInputWidth prop on NumberControl rather than a CSS override?
There was a problem hiding this comment.
That seems to work well for me when testing in Storybook. Was 6fec45f along the lines you were thinking? Just to make sure I've implemented it properly.
If so, it has the added benefit of simplifying all of that inputWidth logic and typing!
|
Looking good! |
|
Thanks @mirka and @richtabor! Lena, I know you dropped a green checkmark after your last review but I've made a few changes based on your feedback, so I'll hold up for a final 👍 and then I'll update the changelog and get this merged. |
mirka
approved these changes
Jun 21, 2023
There was a problem hiding this comment.
Looks good, thank you!
It's a bit weird that rangeHeightValue = 30 is still always used for some margin-top calculations, and ideally that should be refactored to make sense, but I don't consider that a blocker for this PR. Especially since a new RangeControl component is in the pipeline.
|
This PR likely needs a rebase/merge from trunk to get e2e to pass |
sethrubenstein
pushed a commit
to pewresearch/gutenberg
that referenced
this pull request
Jul 13, 2023
…#49105) Co-authored-by: chad1008 <[email protected]>
ciampo
added
the
has dev note
|
For the dev note, see #46741 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Add support for the
size="__unstable-large"prop within the RangeControl component, to support the effort towards consistent components across the board re #46734.Related to #49101 and #46741.
Why?
Support the consistent component effort in #46734.
How?
size(20).size="__unstable-large"to the Cover block's Overlay Opacity control for testing.Testing Instructions
Screenshots or screencast
Before:
After: